Js Support#250
Conversation
This reverts commit 0c78ac8.
wellingtoncosta
left a comment
wellingtoncosta
left a comment
There was a problem hiding this comment.
Left some comments. LGTM overall.
wellingtoncosta
left a comment
wellingtoncosta
left a comment
There was a problem hiding this comment.
I tested locally and it looks good to me.
DanielSouzaBertoldi
left a comment
DanielSouzaBertoldi
left a comment
There was a problem hiding this comment.
Good direction overall, most of the comments are really minor stuff, but there are some of them I think it's really important that we address before we merge. Great work! 🚀
| lifecycle.whileAttached { | ||
| try { | ||
| syncService.runSynchronization(conference = conference) | ||
| syncService.forceSynchronize(conference) |
There was a problem hiding this comment.
This seems like dead code to me 🤔
While reading the runSynchronization code, I noticed it suspends inside a coroutineScope { launch { while(isActive) } } and it just never returns while the scope is alive.
There was a problem hiding this comment.
Also, digging a bit deeper, I believe that we don't even need this forced synchronization.
runSynchronization instantiates the lastSessionizeSyncThisLoop with the current time - 3 hours:
var lastSessionizeSyncThisLoop: Instant = dateTimeService.now().minus(3, DateTimeUnit.HOUR)Which a bit down below timeToSync checks if lastSessionizeSyncThisLoop is equals or more than 15 minutes ago:
// simplified
val timeToSync = lastSessionizeSyncThisLoop <= dateTimeService.now().minus(15, DateTimeUnit.MINUTE)Which means that runSynchronization will always trigger a sync when it first runs.
What do you think? Is there a corner case of some sort I'm not aware of?
There was a problem hiding this comment.
I think there was a corner case but I don't remember at this time. Let me play around with it and see if there's an issue.
| additionalModules + | ||
| platformModule + | ||
| coreModule, | ||
| platformModule + | ||
| coreModule + | ||
| additionalModules, | ||
| ) |
There was a problem hiding this comment.
Why did we reverse the order here? Can't this affect Android/iOS too?
There was a problem hiding this comment.
I'll revert this and see if it works. It might've been based on some of the SQL koin work I'm reverting.
| ), | ||
| conferenceTableAdapter = ConferenceTable.Adapter( | ||
| conferenceTimeZoneAdapter = timeZoneAdapter, | ||
| // Note: selectedAdapter will be added when the adapter is regenerated |
There was a problem hiding this comment.
When exactly is the adapter going to be regenerated? Without it the conference table might not deserialize correctly on web right?
There was a problem hiding this comment.
I'm not sure. It's an object so maybe it won't? I'm not sure exactly what you mean sorry
There was a problem hiding this comment.
I was asking because of the comment "selectedAdapter will be added when the adapter is regenerated", which to me looks like some leftover work? Idk, I'm not sure either what it means lol
There was a problem hiding this comment.
Yea it was a leftover, sorry.
DanielSouzaBertoldi
left a comment
DanielSouzaBertoldi
left a comment
There was a problem hiding this comment.
Good direction overall, most of the comments are really minor stuff, but there are some of them I think it's really important that we address before we merge. Great work! 🚀
Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…/session/SessionDetailViewModel.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…/session/SessionDayViewModel.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…/session/BaseSessionListViewModel.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…mposeView.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…ce/impl/DefaultApiDataSource.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…ce/impl/DefaultApiDataSource.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…ce/impl/DefaultApiDataSource.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
…ce/impl/DefaultApiDataSource.kt Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
Co-authored-by: Daniel Bertoldi <dsdanielbertoldi@gmail.com>
3 participants
Web Support
This PR adds Web support for Droidcon.
How to run:
./gradlew web:jsBrowserDevelopmentRunTo test local times, you need to set
testNotificationTimes = trueinshared/src/commonMain/kotlin/co/touchlab/droidcon/domain/service/impl/DefaultSyncService.ktScreenshots
I have to fix these